Cherry pick PR #9203: cobalt: Disable resource load throttling#9252
Cherry pick PR #9203: cobalt: Disable resource load throttling#9252cobalt-github-releaser-bot wants to merge 4 commits intomainfrom
Conversation
Cobalt operates as a single-tab application, which eliminates the need for resource load throttling. In multi-tab browser environments, throttling can manage resources or prioritize active tabs, but in Cobalt, it provides no benefit and may hinder performance. This change configures ResourceLoadScheduler::IsClientDelayable to always return false for Cobalt builds. This ensures that all resource requests are treated as non-delayable and processed without artificial delays. Bug: 484394054 (cherry picked from commit 0470089)
|
Caution There were merge conflicts while cherry picking! Check out cherry-pick-main-9203 and fix the conflicts before proceeding. Check the log at https://github.com/youtube/cobalt/actions/runs/22411131814 for details. |
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a cherry-pick that disables resource load throttling for Cobalt builds. The implementation in resource_load_scheduler.cc correctly uses a build flag to modify behavior for Cobalt, which is appropriate. However, the corresponding change in BUILD.gn unconditionally comments out a test file. This could lead to a loss of test coverage for non-Cobalt builds if the codebase is used in other contexts. I've left a comment with a suggestion for a more robust, conditional approach to maintain test integrity across different build configurations.
| # resource_load_schedule is disabled, see b/484394054 | ||
| # "fetch/resource_load_scheduler_test.cc", |
There was a problem hiding this comment.
There's a minor typo in the comment: schedule should be scheduler.
More importantly, commenting out this test file removes it unconditionally for all builds. If this source tree is only ever used for Cobalt builds, this is acceptable. However, if there's a possibility of non-Cobalt builds, this will silently disable tests and could hide regressions.
A more robust method would be to conditionally exclude the file using the is_cobalt build flag (or its equivalent) in GN. For example:
if (!is_cobalt) {
sources += [ "fetch/resource_load_scheduler_test.cc" ]
}This would ensure tests run for non-Cobalt configurations. Please consider this for better long-term maintenance.
There was a problem hiding this comment.
I agree that it would be better to conditionally exclude the file using is_cobalt. Can you instead please add something like this around line 263?
if (is_cobalt) {
# resource_load_schedule is disabled, see b/484394054
sources -= [ "fetch/resource_load_scheduler_test.cc" ]
}
hlwarriner
left a comment
hlwarriner
left a comment
There was a problem hiding this comment.
LGTM aside from the one open comment thread.
| # resource_load_schedule is disabled, see b/484394054 | ||
| # "fetch/resource_load_scheduler_test.cc", |
There was a problem hiding this comment.
I agree that it would be better to conditionally exclude the file using is_cobalt. Can you instead please add something like this around line 263?
if (is_cobalt) {
# resource_load_schedule is disabled, see b/484394054
sources -= [ "fetch/resource_load_scheduler_test.cc" ]
}
4 participants
Refer to the original PR: #9203
Cobalt operates as a single-tab application, which eliminates the
need for resource load throttling. In multi-tab browser environments,
throttling can manage resources or prioritize active tabs, but in
Cobalt, it provides no benefit and may hinder performance.
This change configures ResourceLoadScheduler::IsClientDelayable to
always return false for Cobalt builds. This ensures that all resource
requests are treated as non-delayable and processed without artificial
delays.
Bug: 484394054